Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

404 describe #3104

Merged
merged 15 commits into from
Jun 20, 2024
Merged

404 describe #3104

merged 15 commits into from
Jun 20, 2024

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Jun 18, 2024

Fixes #3092
Fixes #3100

  • Disable Pagination (i.e., limited to 500 rows of query output in window)
  • Disable Drill Down (i.e., right-click Pivot to Values)
  • Disable Enhanced Error Reporting (e.g., red squiggly underlines for syntax errors in the editor)
  • Disable Correlations views in Details side panel & window

The app in this PR does a feature detection step whenever the lake changes or when it boots up for the first time. It will check if the describe endpoint returns a 404. If so, it will disable the following checkboxes above.

Everything seems to be working as intended in this.

@@ -27,6 +27,7 @@ export function InitLake({children}) {
useLayoutEffect(() => {
if (status) return
if (lake) dispatch(updateStatus(lake.id))
Active.lake.sync()
Copy link
Member Author

@jameskerr jameskerr Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever the current lake id changes, or it's connection status changes, sync what info we have with the lake server.

@@ -35,7 +36,7 @@ export function InitLake({children}) {

switch (status) {
case "disconnected":
return <ConnectionError onRetry={() => dispatch(initCurrentTab())} />
return <ConnectionError onRetry={() => dispatch(updateStatus(lake.id))} />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initCurrentTab only called this updateStatus function. So I inlined it and removed initCurrentTab.

All of this "lake" status/authentication code needs another look at some point.


export function initLake(store: Store) {
const lakeId = Current.getLakeId(store.getState())
const id = lakeId || defaultLake().id
// This sets up the lake's tabs
const lake = Lake.find(id)
lake.sync()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as the app boots up, we sync the lake.

api.init(store.dispatch, store.getState)
initDOM()
initIpcListeners(store)
initDomainModels({store})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Domain Models need to be provisioned with the store variable before we call "initLake". So I re-ordered the steps here.

@@ -6,6 +6,9 @@ export type LakeAttrs = {
version?: string
authType: AuthType
authData?: AuthData
features?: {
describe: boolean
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add a "features" property the the data we cache about the lake. The only feature right now will be "describe".

static get lake() {
const id = this.select(Current.getLakeId)
return Lake.find(id)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a shortcut to get the Active lake object. Active.lake


type LakeModelAttrs = LakeAttrs & {status: LakeStatus}

export class Lake extends DomainModel<LakeModelAttrs> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are adding the methods we need to the Lake model.

I've added:

  • find(id)
  • client
  • features
  • sync()
  • update
  • save
  • isRunning()

@@ -0,0 +1,21 @@
import {Lake} from "../lake"

export class FeatureDetector {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is responsible for detecting all the features we want.

@@ -65,6 +65,11 @@ export class SessionPageHandler extends ViewHandler {
const program = this.select(Current.getQueryText)
const history = this.select(Current.getHistory)

if (!Active.lake.features.describe) {
Copy link
Member Author

@jameskerr jameskerr Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And finally, if the lake does not have describe, do the minimum required and return early.

});
return result.json();
if (response.ok || response.status === 400) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-arranged the code to allow for an error response from the server. If the describe endpoint returns code 400, there is a problem with the query, but we don't want to throw an error. We want to report the problem to the user. So we parse the JSON as an expected response.

duplex?: 'half';
dontRejectError?: boolean;
}) {
protected request(opts: Types.RequestOpts) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated the creation of the request into this request method. The existing send method then calls this.

The describeQuery method only uses request and handles errors in a different way than the other methods.

@jameskerr jameskerr requested review from philrz and mattnibs and removed request for philrz June 18, 2024 23:35
@philrz

This comment was marked as resolved.

@philrz
Copy link
Contributor

philrz commented Jun 20, 2024

I'm currently testing this branch at commit 62a6084.

The attached video shows a successful walk-through of the functionality that these changes disable when accessing an older Zed lake service. As prep, I have the ZNG format Zed sample data loaded to both the lake running behind Zui (Zed commit 13d15d3) and a separate "remote" Zed v1.14.0 lake I've started on port 8888. As a baseline I first demo each of the four functional areas described by the checkboxes in the opening PR text while accessing the data in the local lake, then I access the v.14.0 lake and confirm that each of these is disabled.

Demo.mp4

@jameskerr jameskerr merged commit 10d8974 into main Jun 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants